Skip to content

HYPERFLEET-619 - fix: Prevent duplicate nodepool names within a cluster#53

Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift-hyperfleet:mainfrom
Mischulee:HYPERFLEET-619
Feb 16, 2026
Merged

HYPERFLEET-619 - fix: Prevent duplicate nodepool names within a cluster#53
openshift-merge-bot[bot] merged 1 commit intoopenshift-hyperfleet:mainfrom
Mischulee:HYPERFLEET-619

Conversation

@Mischulee
Copy link
Contributor

@Mischulee Mischulee commented Feb 11, 2026

https://issues.redhat.com/browse/HYPERFLEET-619

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where duplicate node pool names could be created within the same cluster. The system now properly validates and rejects attempts to create node pools with duplicate names, returning an appropriate error response.

@coderabbitai
Copy link

coderabbitai bot commented Feb 11, 2026

Walkthrough

The changes add database-level enforcement of unique node pool names per cluster and corresponding test coverage. A migration creates a unique index on node_pools(owner_id, name) filtered for non-deleted records. A package-level constant kindNodePool replaces hard-coded strings in tests, and a new integration test TestNodePoolDuplicateNames validates that attempting to create a second node pool with an existing name in the same cluster returns a 409 Conflict error with proper error payload fields.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

lgtm, approved

Suggested reviewers

  • 86254860
  • jsell-rh
🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Merge Conflict Detection ⚠️ Warning ❌ Merge conflicts detected (6 files):

⚔️ charts/values.yaml (content)
⚔️ docs/api-resources.md (content)
⚔️ pkg/db/migrations/202511111055_add_node_pools.go (content)
⚔️ pkg/handlers/cluster_status.go (content)
⚔️ test/integration/adapter_status_test.go (content)
⚔️ test/integration/node_pools_test.go (content)

These conflicts must be resolved before merging into main.
Resolve conflicts locally and push changes to this branch.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: preventing duplicate nodepool names within a cluster, which is directly supported by the database migration adding a unique index and the new integration test validating the constraint.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
⚔️ Resolve merge conflicts (beta)
  • Auto-commit resolved conflicts to branch HYPERFLEET-619
  • Post resolved changes as copyable diffs in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/api/node_pool_types.go`:
- Line 17: Remove the redundant GORM uniqueIndex tags from the NodePool struct:
delete `uniqueIndex:idx_owner_name` from the Name field's struct tag and from
the OwnerID field's struct tag; retain the `index` tag on OwnerID if
single-column indexing is desired, since the canonical unique constraint is
defined in the migration `202602111400_add_nodepool_unique_name_constraint.go`
and migrations are the source of truth.
🧹 Nitpick comments (2)
test/integration/node_pools_test.go (1)

326-359: Good test coverage for the duplicate name conflict scenario.

The test correctly verifies that duplicate names within the same cluster return a 409 Conflict. Consider adding additional test cases to ensure complete coverage of the uniqueness constraint:

  1. Cross-cluster duplicate names should succeed - Verify that the same name can be used in different clusters
  2. Name reuse after soft-delete - Verify that deleting a nodepool allows its name to be reused (tests the WHERE deleted_at IS NULL partial index)

These would validate both the positive and edge-case behaviors of the constraint.

pkg/db/migrations/202602111400_add_nodepool_unique_name_constraint.go (1)

8-28: Consider pre-existing duplicate data before migration.

If the database already contains duplicate (owner_id, name) pairs for active nodepools, this migration will fail. Consider adding a pre-migration check or documenting the remediation steps.

💡 Suggested improvement: Add a pre-check query
 func addNodePoolUniqueNameConstraint() *gormigrate.Migration {
 	return &gormigrate.Migration{
 		ID: "202602111400",
 		Migrate: func(tx *gorm.DB) error {
+			// Check for existing duplicates before creating the index
+			var count int64
+			checkSQL := `SELECT COUNT(*) FROM (
+				SELECT owner_id, name FROM node_pools 
+				WHERE deleted_at IS NULL 
+				GROUP BY owner_id, name HAVING COUNT(*) > 1
+			) AS duplicates`
+			if err := tx.Raw(checkSQL).Scan(&count).Error; err != nil {
+				return err
+			}
+			if count > 0 {
+				return fmt.Errorf("cannot create unique constraint: %d duplicate (owner_id, name) pairs exist", count)
+			}
+
 			// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
 			createIndexSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
 				"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"

Note: You would also need to add "fmt" to the imports.

@@ -0,0 +1,29 @@
package migrations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add this code to the existing add_node_pools file?

The reason being that we don't need to keep compatibility with older database schema yet, so it will be better to have less migration files at the begining.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. As we have not yet exposed/released our framework, we are still at the first version, so we do not need the extra migration file, but instead, integrating in the current one is a better choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, fixed.

@@ -0,0 +1,29 @@
package migrations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. As we have not yet exposed/released our framework, we are still at the first version, so we do not need the extra migration file, but instead, integrating in the current one is a better choice.


// Owner references (expanded)
OwnerID string `json:"owner_id" gorm:"size:255;not null;index"`
OwnerID string `json:"owner_id" gorm:"size:255;not null;index;uniqueIndex:idx_owner_name"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As same as above.

ctx, cluster.ID, openapi.CreateNodePoolJSONRequestBody(nodePoolInput), test.WithAuthToken(ctx),
)
Expect(err).NotTo(HaveOccurred())
Expect(resp.StatusCode()).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider also asserting the response body for the 409 case, not just the status code. Per the
HyperFleet error model standard (RFC 9457 Problem Details), the response should include code:
"HYPERFLEET-CNF-001", a meaningful detail message, and the standard type/title fields.
Validating the body ensures the error message is user-friendly and prevents regressions in the
error format.

For example:

  // After asserting StatusCode == 409:
  Expect(resp.JSON409).NotTo(BeNil())
  // or parse the body and check:
  // Expect(problemDetail.Code).To(Equal("HYPERFLEET-CNF-001"))
  // Expect(problemDetail.Detail).To(ContainSubstring("already exists"))

Copy link
Contributor Author

@Mischulee Mischulee Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertions to validate the error response body added.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/db/migrations/202511111055_add_node_pools.go`:
- Around line 66-71: Before executing tx.Exec(createUniqueIdxSQL) to create
idx_node_pools_owner_name, run a preflight duplicate check against node_pools
for rows with deleted_at IS NULL (e.g., SELECT owner_id, name, COUNT(*) FROM
node_pools WHERE deleted_at IS NULL GROUP BY owner_id, name HAVING COUNT(*) > 1)
using the same tx; if the query returns any rows, return a clear error from the
migration that lists the offending owner_id/name (and counts) or perform an
explicit cleanup/deduplication step, and only proceed to execute
createUniqueIdxSQL when no duplicates are found.

Comment on lines +66 to +71
// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
createUniqueIdxSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"
if err := tx.Exec(createUniqueIdxSQL).Error; err != nil {
return err
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Handle existing duplicate nodepool names before creating the unique index.

Line 66–69 will fail the migration if any existing (owner_id, name) duplicates exist among non-deleted rows, which can block upgrades on live clusters. Consider a preflight duplicate check with a clear error (or a cleanup step) before creating the index.

🧪 Suggested preflight check (with explicit error)
 import (
+	"fmt"
 	"gorm.io/gorm"

 	"github.com/go-gormigrate/gormigrate/v2"
 )
@@
 			// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
+			type dupRow struct {
+				OwnerID string
+				Name    string
+				Count   int64
+			}
+			var dupes []dupRow
+			if err := tx.Raw(`
+				SELECT owner_id, name, COUNT(*) AS count
+				FROM node_pools
+				WHERE deleted_at IS NULL
+				GROUP BY owner_id, name
+				HAVING COUNT(*) > 1;
+			`).Scan(&dupes).Error; err != nil {
+				return err
+			}
+			if len(dupes) > 0 {
+				return fmt.Errorf("duplicate nodepool names exist: %+v", dupes)
+			}
 			createUniqueIdxSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
 				"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
createUniqueIdxSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"
if err := tx.Exec(createUniqueIdxSQL).Error; err != nil {
return err
}
import (
"fmt"
"gorm.io/gorm"
"github.com/go-gormigrate/gormigrate/v2"
)
// ... within the migration function ...
// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
type dupRow struct {
OwnerID string
Name string
Count int64
}
var dupes []dupRow
if err := tx.Raw(`
SELECT owner_id, name, COUNT(*) AS count
FROM node_pools
WHERE deleted_at IS NULL
GROUP BY owner_id, name
HAVING COUNT(*) > 1;
`).Scan(&dupes).Error; err != nil {
return err
}
if len(dupes) > 0 {
return fmt.Errorf("duplicate nodepool names exist: %+v", dupes)
}
createUniqueIdxSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"
if err := tx.Exec(createUniqueIdxSQL).Error; err != nil {
return err
}
🤖 Prompt for AI Agents
In `@pkg/db/migrations/202511111055_add_node_pools.go` around lines 66 - 71,
Before executing tx.Exec(createUniqueIdxSQL) to create
idx_node_pools_owner_name, run a preflight duplicate check against node_pools
for rows with deleted_at IS NULL (e.g., SELECT owner_id, name, COUNT(*) FROM
node_pools WHERE deleted_at IS NULL GROUP BY owner_id, name HAVING COUNT(*) > 1)
using the same tx; if the query returns any rows, return a clear error from the
migration that lists the offending owner_id/name (and counts) or perform an
explicit cleanup/deduplication step, and only proceed to execute
createUniqueIdxSQL when no duplicates are found.

$ref: '#/components/schemas/NodePoolCreateResponse'
'400':
description: The server could not understand the request due to invalid syntax.
'409':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This openapi.yaml is a copy from the one we generate with the https://github.com/openshift-hyperfleet/hyperfleet-api-spec repository.

This means that if we don't make this change to that repository, the change will be lost the next time we update the api-spec and copy over this repository.

If we want this 409 to follow the schema, it may be a good opportunity to go to the api-spec and review the 409 responses in other cases, since improving the error handling in the schema is still something we have to work on.

So, I think we should either:

  • Keep this change but also change the api-spec
  • Discard this change here, and open a ticket to review the api-spec error schemas

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rh-amarin Oh, I forgot that openapi.yaml is generated. What about using resp.ApplicationproblemJSONDefault() instead of a specific ApplicationproblemJSON409() for general errors for now, so we dont need to update the api-spec yet? We could then review and improve the error-handling schema.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is option 2
Enhancing API spec and error handler deserves its own spike/epic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rh-amarin Done

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/integration/node_pools_test.go (1)

327-370: Optional: add a “same name, different cluster” success case.

This would prove uniqueness is scoped to the cluster, not globally.

➕ Suggested test extension
@@
 	Expect(*problemDetail.Detail).To(ContainSubstring("already exists"),
 		"Expected error detail to mention that resource already exists")
+
+	// Optional: same name should be allowed in a different cluster
+	cluster2, err := h.Factories.NewClusters(h.NewID())
+	Expect(err).NotTo(HaveOccurred())
+	resp, err = client.CreateNodePoolWithResponse(
+		ctx, cluster2.ID, openapi.CreateNodePoolJSONRequestBody(nodePoolInput), test.WithAuthToken(ctx),
+	)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(resp.StatusCode()).To(Equal(http.StatusCreated))
 }

@rh-amarin
Copy link
Contributor

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Feb 16, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rh-amarin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 9d73fbd into openshift-hyperfleet:main Feb 16, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants